IonExchange0D Unit Model and Costing Improvements #1139
Conversation
kurbansitterley
added
the
model dev team
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1139 +/- ##
=======================================
Coverage 94.27% 94.27%
=======================================
Files 357 357
Lines 36011 35981 -30
=======================================
- Hits 33950 33922 -28
+ Misses 2061 2059 -2 ☔ View full report in Codecov by Sentry. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
No quarrels with the costing changes. I do have a question regarding the single_use condition which a lot of the changes are surrounding the conditional with this port. Is the regen port containing solutes, resin, or both? See other comments regarding mass balance perspective.
| @@ -80,7 +83,7 @@ The model provides three ports (Pyomo notation in parenthesis): | |||
|
|
|||
| * Inlet port (inlet) | |||
| * Outlet port (outlet) | |||
| * Regeneration port (regen) | |||
| * Regeneration port (regen, only if not using ``single_use`` resin configuration) | |||
There was a problem hiding this comment.
Its slightly confusing from a mass balance perspective. If set to single_use, inlet == outlet so how is mass removed from a modeling perspective? There is no spent resin stream that would then be regenerated with x efficiency and then added as the regen stream, correct?
There was a problem hiding this comment.
Hm maybe a poor choice of words on my part. The mass balance is handled the same as before where effluent concentration is estimated via trapezoid rule, which is then used to set process_flow.mass_transfer_term (see e.g., eq_mass_transfer_target_fr - I think this is pretty much identical to how GAC does it) . Without single-use, that flows to the regeneration_stream but with single-use it goes nowhere. There was never any regeneration efficiency in the model, so anything that flowed to that port was assumed to be removed via regeneration (which is not handled by the model; a user could attach a separate unit process as a regen handling process if they wanted.
But maybe there is virtue in having the removed mass flow somewhere rather than a black hole.
There was a problem hiding this comment.
I remember this 'into the void' method when originally developing the model.
I think I'm still confused that if the contaminants are going 'into the void' and the resin is also not modeled with a pseudo-species method, what is the intention of the regeneration_stream? i.e., if someone wanted to customize a regen or waste handling unit, what is this stream/port accomplishing?
There was a problem hiding this comment.
The regeneration_stream could be used to connect a brine/regen management unit to it, as you say... but it is a good point; I imagine the bigger challenge in managing IX regen streams is the high salinity/concentration of regenerant they typically contain rather than the flow of removed species, so one could argue the regeneration stream shoudl contain the regenerant rather than the target_ion (if it is to contain anything). But that would require some more development to accomplish and likely carries its own headaches...
I'll have to think about this one.
There was a problem hiding this comment.
Following up- I added the regeneration_stream into the model for all CONFIGs (i.e. undoing what I had initially implemented). I also added back backwashing/rinsing costs/params/expressions for the single_use option. Now only regenerant costs are excluded with the single_use configuration.
|
@kurbansitterley I know you probably realized already, but there is a merge conflict likely due to the reformulation/movement of costing files in #1122 |
|
@kurbansitterley Is this waiting on anything other than review? I will review again if these failing checks are workflow incidental. |
Yes I wanted to wait for #1175 to be merged since it also changes the IX costing. |
ksbeattie
added
Priority:High
|
@kurbansitterley let me know if you want assistance resolving the conflicts post-#1175. |
| assert value(m.fs.ion_exchange.partition_ratio) == pytest.approx( | ||
| 235.99899, rel=1e-3 | ||
| ) | ||
| results_dict = { |
There was a problem hiding this comment.
This is pretty unsightly and could get cleaned up. That is why we use approx after all, I think I include 4 significant figures and pytest to 1e-3. This applies to a lot of code across the test files.
There was a problem hiding this comment.
Don't disagree, was trying to move fast. Ask and ye shall receive.
Fixes/Resolves:
single_useoption for regeneration and associated costingbed_capacity_paramvariable and associated constraints to improve model stability/solvability.Summary/Motivation:
Changes to model made as results of continuing PFAS analysis.
bed_capacity_paramwas found to explode to massive values (>1e+20) under many model conditions, resulting in unbounded/infeasible solves (or optimal solves that were non-sensical). This PR removes that variable and all associated constraints and expressions from the model. The only place this variable was found to be necessary was in the calculation of estimated breakthrough times for the trapezoid approach used for steady-state effluent concentrationtb_traps. A suitable replacement was used (essentially theeq_clark_1constraint)Changes proposed in this PR:
bed_capacity_param,kinetic_paramand associated constraints/expressionssingle_useCONFIG option that assumes entire bed is replaced rather than regeneratedLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: